Allow message body redaction #67
Conversation
mdlavin
force-pushed
the
allow-message-body-redaction
branch
from
447fd43 to
d509ed9
Compare
|
Woah, only after creating this did I see - #61 (comment). I'd love to hear @aecorredor's thoughts about this |
There was a problem hiding this comment.
I like that we now cover SQS. The negative side is that we now have two solutions for the same thing: one in the dynamo stream handler and one for the SQS one. It'd be great to be able to consolidate.
I think we could make SQS safe now and come back to consolidate later though.
src/sqs.ts
Outdated
| try { | ||
| return redactor(body); | ||
| } catch (error) { | ||
| logger.error({ error, body }, 'Failed to redact message body'); |
There was a problem hiding this comment.
If this did in fact include PHI and someone wanted to redact, but did something wrong, we'll still end up logging the PHI. Does it make sense to just not log the body here?
|
I approved, but I realized that logging the body in the catch might actually be worth removing. |
I left a comment about that in the testcase. What do you think of my logic? @aecorredor |
src/sqs.test.ts
Outdated
| // unredacted error allows for easier debugging. Leaking a small amount of | ||
| // PHI to Sumo is better than having a system that cannot be debugged. |
There was a problem hiding this comment.
Ah, I did miss this. Sorry. If the tradeoff is legally acceptable, I'm all for it. Easier debugging is always better.
There was a problem hiding this comment.
Legally I think we're fine. The biggest risk I can think of would be constantly failing redaction logic that logs lots of data and we don't notice. An alternative would be to completely fail and to log the unredacted body. That would force a fix while giving enough data to debug when needed
There was a problem hiding this comment.
@swain @indigocarmen What do you think?
There was a problem hiding this comment.
What do you have in mind for when you say "completely fail". Since this happens in an SQS handler, it retries based on the queue settings.
There was a problem hiding this comment.
I mean throw an exception without handling the message rather than keep going. Throwing the exception would mean that it would be hard to ignore the failure to redact. There would be a small amount of PHI logged for that one message. By logging a warning, it would be pretty easy to ignore and then have the logs full of PHI over many messages.
There was a problem hiding this comment.
Hmm...I think if a developer has explicitly provided logic for redacting data from the message body, it seems like we should not definitely not log that data. Logging a small amount of PHI seems like it is theoretically not okay, right?
@mdlavin It seems like your goal in including that log is supporting debugging. Could we log the SQS message id for that particular record instead of the full body?
There was a problem hiding this comment.
Logging a small amount of PHI seems like it is theoretically not okay
I think it's fine as an edgecase. Sumo can hold PHI and we want to minimize it.
Could we log the SQS message id for that particular record instead of the full body?
How does a developer debug with that?
There was a problem hiding this comment.
Could we log the SQS message id for that particular record instead of the full body?
How does a developer debug with that?
Good question. Thinking deeper, I couldn't come up with a solid path.
I think it's fine as an edgecase. Sumo can hold PHI and we want to minimize it.
I think I disagree about having the behavior in this package, but I'm willing to commit. You're the first user of the feature, and it seems like you feel like it is important and will be useful.
There was a problem hiding this comment.
It does seem like it should be possible to create a redacted log message that does contain PHI, but is still useful for debugging (e.g. contains ids, etc). But, approving to trust your judgement.
There was a problem hiding this comment.
I just pushed a new fix. Here is how the code should work:
- If no redaction is enabled then the event body is logged as is
- If redaction is enabled and it does not error, then the redacted event body is logged
- If redaction is enabled and it fails, then the event is logged with the whole body repalced with 'REDACTED' and an RSA encrypted blob holding the body to be used for debugging. Devs could keep a private key to use for accessing the details when critical
- If redaction fail, and encryption fails, then the body is logged as 'REDACTED', the encryption failure is logged. No way to debug from this. Need to deliver a code fix to repair the failed encryption
It's the best mix of security + developer experience I could come up with. Sensitive data is unlikely to be leaked by accident and developers can get access to the failed event body by jumping through some manual steps of RSA decryption.
|
🎉 This PR is included in version 5.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Some messages from the member ops bot will contain PHI. I'd like to redact that from the messages instead of logging it.